-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add flux-corrected transport scheme to MALI #70
Add flux-corrected transport scheme to MALI #70
Conversation
Add an option to use the flux-corrected transport advection scheme provided in MPAS framework, and make calls to those modules from mpas_li_advection. Compiles, but not yet tested. No expectation that it will run yet.
We have decided to use modified versions of the routines for higher-order advection from MPAS-Ocean. This commit copies over the modules from the ocean model, but does not modify or call them yet.
Update Makefile to include fct modules. Currently does not compile because of undefined variables.
Initialize fct from tracer_setup. Also rename public routines to start with "li_"
Add li_mesh module that is called during init to make all mesh parameters public. This is copied from mpas-ocean, with modifications appropriate for MALI.
Comment out irrelevant parameters and routine in li_mesh that are specific to MPAS-Ocean and caused build errors.
Add li_config module so other modules can get all configs with 'use li_config'
Also general cleanup to get rid of build errors stemming from these modules.
Some fields need to be revisited for accuracy, but these changes fix build errors.
Remove call to tracer_advection_vert_flx from li_tracer_advection_fct_tend. We will probably want to add this back, but for now just trying to get this to compile without vertical fct. And now it compiles!
Code finally compiles as of 414eac6! |
Differences between ocean and land ice mesh variables were causing segmentation faults at run time. This clean-up commit allows full_integration to run and pass, which does not test the fct code at all, but shows that li_mesh is now operational.
|
Fix issues with public parameter allocations. This now allows the code to run using config_tracer_advection='fct' without invalid memory reference errors, although results are not yet valid.
Clean up li_advection_fct_tend to get rid of runtime errors. All vertical advection code is currently commented out and not operational. This runs with config_thickness_advection='fo' and config_tracer_advection='fct', but it currently seems to give the same results for the humboldt 3km test case in full_integration as using 'fo' for both thickness and tracer advection. Need to look further into that.
Currently fct advection for thickness is not supported, but fo thickness advection wtih fct tracer advection is operational, although not validated.
Testing on a364f7a shows that the code will run with |
Previous commit neglected to actually update tracers after calculating tendencies. This corrects that.
As of e85d3fd, fct tracer advection is at least doing something, although is not yet verified. Here are three MISMIP runs after 100 years with velo solver turned off, no sub-shelf melt, calculate-damage = true. All use 'fo' thickness advection. Left: no tracer advection; Middle: fo tracer advection; right: fct tracer advection: |
Pass layerThicknessOld instead of layerThickness to fct tracer advection routine because the fct routine calculates a provisional advected layerThickness based on the normalThicknessFlux, which is layerThicknessEdge (before advection) * layerNormalVelocity.
Add support for fct thickness advection by passing layerThickness to li_tracer_advection_fct_tend as a tracer. Note that this seems to require small time steps to avoid terracing effects in the thickness field.
As of 8734303, fct thickness advection is operational, and seems to be working correctly. One issue is that it seems to require a very small CFL fraction to avoid terracing effects. CFL fraction of 0.1 (~40 day time steps) seems to be sufficiently low here. I tested another run w/ a CFL fraction of 0.01 (~4 days) and the differences at year 100 are generally <1 m: What we are seeings seems to be a known issue w/ FCT: "Another infamous byproduct of FCT manifests itself in distortions of a smooth profile. This phenomenon is known as terracing and represents ‘an integrated, nonlinear effect of residual phase errors’ [64] or, loosely speaking, ‘the ghosts of departed ripples’ [7]. A particularly severe form of terracing is caused by the linear instability of the high-order scheme. For this reason, we do not recommend the use of the forward Euler time-stepping (θ = 0) even though the flux-corrected scheme proves positivity-preserving under the CFL-like time step restriction (62)." |
Testing on Thwaites mesh indicates issue with FCT thickness advection during advance, which is not surprising. It turns out that |
First order flux needs to be masked when subtracted from higher order flux, because they don't have the same valid spatial extent. This seems to largely fix the issue with thickness ramps growing at the ice margin in tests on the Thwaites domain. It also leads to a bit more diffusion of the ice mask in the slotted cylinder test, but it also gets rid of the noisy artifacts in the thickness field, so that's great!
Uh oh, it appears that Both runs do conserve mass, however. Blue is fo, orange is fct: Update: |
Fix a bug in fct tracer advection that caused tracers to not be conserved due to an inconsistent treatment of the thickness flux. The previous implementation inadvertently used first-order upwind thickness flux to calculate higher-order tracer flux, which led to loss of conservation when both thickness and tracers used fct. Note that this still does not conserve tracers to machine precision, even though mass is conserved to machine precision. But testing shows the tracer volume conserved to within about 0.02%.
8d2f023 fixed the tracer conservation issue detailed above! It still doesn't conserve to machine precision, but a 100 year test run on the grounded slab test case showed about 0.06% drift by the end of the simulation. That seems pretty close! |
@dengwirda, see note about slight non-conservation of tracers in Trevor's current implementation (#70 (comment)). Should we be expecting machine precision conservation? If so, do you think we should worry that we're not achieving that given how small the non-conservation is? I suppose we would be introducing energy conservation errors, given that we will be advecting temperature with this scheme. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@trhille , I've made it through these changes and have a number of requests and questions to address, but this seems very close to being ready to merge. Thanks for you hard work and what turned out to be a much more involved project than initially anticipated!
components/mpas-albany-landice/src/mode_forward/mpas_li_advection.F
Outdated
Show resolved
Hide resolved
components/mpas-albany-landice/src/mode_forward/mpas_li_advection.F
Outdated
Show resolved
Hide resolved
! Determine bounds on tracer (tracerMin and tracerMax) from | ||
! surrounding cells for later limiting. | ||
|
||
#ifdef MPAS_OPENACC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove accelerator stuff?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We decided to save removing accelerator stuff for a separate PR.
! This does conserve mass: | ||
layerThickness(:,:) = layerThickness(:,:) + tend(nTracers,:,:) * dt | ||
! Reset tracers after thickness advection for tracer advection | ||
! TODO: determine whether we need to treat other tracer tendencies |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@trhille , have you looked into this question?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it doesn't work for tracers, for some reason.
components/mpas-albany-landice/src/mode_forward/mpas_li_advection_fct_shared.F
Outdated
Show resolved
Hide resolved
call li_tracer_advection_fct_tend(& | ||
tend, advectedTracers, layerThicknessOld, & | ||
layerThicknessEdge * layerNormalVelocity, 0 * normalVelocity, dt, & | ||
nTracers, computeBudgets=.false.)!{{{ | ||
nTracers, activeTracerHorizontalAdvectionEdgeFlux, computeBudgets=.false.)!{{{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may want to move activeTracerHorizontalAdvectionEdgeFlux
to Registry so it could be output given its importance in the algorithm. But we'd want to refactor things to avoid it being nTracers
big first. So let's wait until we encounter a need to visualize it before worrying about that.
Add explanatory comments, remove commented code, clarify desciriptions in Registry.xml
Throw error forr fct thickness with fo tracer, as this combination is currently not supported.
Make activeTracerHorizontalAdvectionEdgeFlux an optional argument when calling fct code.
This fixes an error introduced in the previous commit that prevented code from compiling.
Fix a bug that disabled thickness advection when tracer advection is turned off.
Only pass layer thickness tracer instead of all tracers for thickness advection step, which is the first call to fct when using fct for both thickness and tracers.
Change passiveTracer to passiveTracer2d
9de6e44
to
8f218c3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@trhille , thanks for all the clean up on this PR. I would like to proceed with merging the version you have now and resolve additional issues related to conservations and convergence in follow up PRs as we learn more.
However, it would be nice to first add the logic to support FCT thickness advection with no tracer advection. That would be useful for the horizontal_advection convergence test using the thickness bump and for the halfar convergence test. I think disabling tracer advection would speed those tests up considerably.
One solution would be to fold it into this option:
elseif ((trim(config_thickness_advection) .eq. 'fct') .and. &
trim(config_tracer_advection) .eq. 'fct') then
and then only do the tracer bit if tracer advection is set to fct.
Enable fct thickness advection without tracer advection and throw error if using unsupported combination of thickness and tracer advection. Also initialize some previously uninitialized allocatable arrays.
358431a
to
52d9a0b
Compare
Testing@trhille , thanks for making the final (more complicated than expected) adjustment. I tested one more time and all tests pass:
I will also test the FCT tests you have in MPAS-Dev/compass#650 |
Testing, Part 2The FCT suite all passes. I did not look at any of the output, but they all run and pass validation. For the sake of merging this PR as a checkpoint, I think that is sufficient.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The long-awaited for day is upon us! Awesome work @trhille !
This merge adds a flux-correct transport (FCT) scheme to MALI for thickness and tracer advection, ported over with MALI-relevant modification from MPAS-Ocean's routines, which are based on Skamarock and Gassmann (2011) (https://doi.org/10.1175/MWR-D-10-05056.1). This uses a blend of 3rd and 4th order fluxes to achieve monotonicity. The FCT routine is only used for tracers in MPAS-Ocean, whereas here it is modified for use with both thickness and tracers. The user can specify 2nd, 3rd, or 4th order advection with the
config_horiz_tracer_adv_order
option, but onlyconfig_horiz_tracer_adv_order = 3
with 0 <config_advection_coef_3rd_order
< 1 is truly FCT. Theconfig_advection_coef_3rd_order
option specifies the blend between 3rd and 4th order fluxes used in the flux correction.config_advection_coef_3rd_order = 1.0
is purely 3rd order, whileconfig_advection_coef_3rd_order = 0.0
is purely 4th order. The default value of 0.25 is taken from the MPAS-Ocean default and may not be appropriate for all situations. Note that all higher-order advection must reduce to 1st order at the boundaries.This also adds a new variable
passiveTracer2d
, that can be used to verify advection schemes.Currently supported combinations of thickness and tracer advection with fct include:
config_thickness_advection = 'fo'
;config_tracer_advection = 'fct'
config_thickness_advection = 'fct'
;config_tracer_advection = 'fct'
config_thickness_advection = 'fct'
;config_tracer_advection = 'none'
FCT tracer advection with no thickness advection and FCT thickness advection with FO tracer advection could be added, but we do not currently anticipate using them. Therefore, we have left them out of this PR as they would add unnecessary complexity to the code.
When used with forward Euler time integration, FCT requires a severely reduced time step relative to first order advection. Testing shows that CFL fraction on the order of 0.1 is likely sufficiently small, but this is probably case-dependent. Once Runge-Kutta time integration is operational, it is recommended to use that instead of forward Euler.
At the time of merging, there is a very slight conservation error (<0.1% in the grounded slab test shown below) for tracers when using FCT for both thickness and tracer advection, while mass is fully conserved. Tracers are conserved when using FO thickness advection with FCT tracer advection. The convergence order has not yet been verified, but testing is under way using a new mesh convergence COMPASS test: MPAS-Dev/compass#690.